Optimize nested streams #33
Conversation
| McpSchema.ErrorCodes.INTERNAL_ERROR, error.getMessage(), null)); | ||
| transport.sendMessage(errorResponse).subscribe(); | ||
| }); | ||
| handleIncomingRequest(request).subscribeOn(Schedulers.boundedElastic()) |
There was a problem hiding this comment.
Why do you think this is necessary? Can you provide a scenario that hinted the issue lies here? This thread hop should definitely not be introduced into DefaultMcpSession as this implementation is agnostic of the runtime and the actual problem and solution must be lying elsewhere.
There was a problem hiding this comment.
I looked at the whole call link and found that there would be no block scene here, so for now there is no need to switch threads, ignore it and I will revert it.
But I found that I could merge streams here.
@chemicL
There was a problem hiding this comment.
Ok got it. Thanks for explaining. There is a draft PR that is aimed at a broader refactor and has a similar optimization, although takes it a step further with no fire-and-forget subscribe: https://github.com/modelcontextprotocol/java-sdk/pull/31/files#diff-4d7a1646fdb5ad78355a228f92ef5883406657076c0a0452675fed2b703fd84cR106-R112
I believe we'll still have DefaultMcpSession after the changes, but let me get back to your PR once we are closer to merging that broader rework.
There was a problem hiding this comment.
No problem. After the draft PR is merged, I will solve some relevant conflicts to complete the PR.
CrazyHZM
force-pushed
the
fix/non-blocking-context
branch
from
7893d2b to
28594fb
Compare
CrazyHZM
changed the title
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context